Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Also syntheize calls to inherited ToString. #18508

Merged

Conversation

michaelnebel
Copy link
Contributor

No description provided.

@michaelnebel michaelnebel force-pushed the csharp/implicitinheritedtostring branch from 6293862 to bae29ae Compare January 16, 2025 15:57
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Jan 20, 2025
@michaelnebel michaelnebel marked this pull request as ready for review January 20, 2025 09:05
@Copilot Copilot bot review requested due to automatic review settings January 20, 2025 09:05
@michaelnebel michaelnebel requested a review from a team as a code owner January 20, 2025 09:05
@michaelnebel
Copy link
Contributor Author

DCA looks good.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • csharp/ql/test/library-tests/implicittostring/implicitToString.expected: Language not supported
  • csharp/ql/test/library-tests/implicittostring/implicitToString.ql: Language not supported
  • csharp/ql/test/query-tests/Nullness/Implications.expected: Language not supported
Comments suppressed due to low confidence (2)

csharp/ql/test/library-tests/implicittostring/implicitToString.cs:49

  • Ensure that the behavior of the inherited ToString method in Container2 is covered by the test cases.
y = "Hello" + container2; // Implicit Container.ToString call.

csharp/ql/test/library-tests/implicittostring/implicitToString.cs:52

  • Ensure that the behavior of the inherited ToString method in Container3 is covered by the test cases.
y = "Hello" + container3; // Implicit Object.ToString call.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.GetMembers()
.OfType<IMethodSymbol>()
.Where(method =>
method.GetName() == "ToString" &&
method.Parameters.Length == 0
)
.FirstOrDefault();

return toString ?? GetToStringMethod(type.BaseType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd that we have no helper for getting the base types already. Maybe adding this to SymbolExtensions would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the base types are used in all sorts of different ways and I didn't find any overlapping use cases for recursively going through the type hierarchy to find a method.

@michaelnebel michaelnebel merged commit ef034bc into github:main Jan 20, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants